-
Notifications
You must be signed in to change notification settings - Fork 78
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Coding guideline] Add implementation and grouping section #641
[Coding guideline] Add implementation and grouping section #641
Conversation
Best reviewed: commit by commit
Optimal code review plan (2 commits squashed)
|
[CHATOPS:HELP] ChatOps commands.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remaining comments which cannot be posted as a review comment to avoid GitHub Rate Limit
LanguageTool
docs/contributing/coding-style.md|460 col 8| Loose punctuation mark. (UNLIKELY_OPENING_PUNCTUATION[1])
Suggestions: ``
Rule: https://community.languagetool.org/rule/show/UNLIKELY_OPENING_PUNCTUATION?lang=en-US&subId=1
Category: PUNCTUATION
docs/contributing/coding-style.md|462 col 3| Don't put a space before the closing parenthesis (COMMA_PARENTHESIS_WHITESPACE)
Suggestions: }
Rule: https://community.languagetool.org/rule/show/COMMA_PARENTHESIS_WHITESPACE?lang=en-US
Category: TYPOGRAPHY
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTMed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
docs/contributing/coding-style.md
Outdated
``` | ||
|
||
Instead of this style. | ||
addr := conn.LocalAddr() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[LanguageTool] reported by reviewdog 🐶
Add a space between sentences (SENTENCE_WHITESPACE)
Suggestions: LocalAddr
Rule: https://community.languagetool.org/rule/show/SENTENCE_WHITESPACE?lang=en-US
Category: TYPOGRAPHY
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry I overlooked this
LGTMed
docs/contributing/coding-style.md
Outdated
<table> | ||
<thead><tr><th>Bad</th><th>Good</th></tr></thead> | ||
<tbody><tr><td> | ||
|
||
```go | ||
// good | ||
if err := fn(); err != nil { | ||
// handle error | ||
} | ||
``` | ||
|
||
Instead of this style. | ||
</td><td> | ||
|
||
```go | ||
// bad | ||
err := fn() | ||
if err != nil { | ||
// handle error | ||
} | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
opposite
/rebase |
[REBASE] Rebase triggered by kpango for branch: documentation/coding_guideline/func_option_and_grouping |
73eab54
to
4fec1e9
Compare
[FORMAT] Updating license headers and formatting go codes triggered by kpango. |
// good | ||
if err := fn(); err != nil { | ||
err := fn() | ||
if err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[LanguageTool] reported by reviewdog 🐶
Don't put a space after the opening parenthesis (COMMA_PARENTHESIS_WHITESPACE)
Suggestions: {
Rule: https://community.languagetool.org/rule/show/COMMA_PARENTHESIS_WHITESPACE?lang=en-US
Category: TYPOGRAPHY
// bad | ||
err := fn() | ||
if err != nil { | ||
if err := fn(); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[LanguageTool] reported by reviewdog 🐶
Don't put a space after the opening parenthesis (COMMA_PARENTHESIS_WHITESPACE)
Suggestions: {
Rule: https://community.languagetool.org/rule/show/COMMA_PARENTHESIS_WHITESPACE?lang=en-US
Category: TYPOGRAPHY
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
// good | ||
conn, err := net.Dial("tcp", "localhost:80") | ||
if err != nil { | ||
if conn, err := net.Dial("tcp", "localhost:80"); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[LanguageTool] reported by reviewdog 🐶
Add a space between sentences (SENTENCE_WHITESPACE)
Suggestions: Dial
Rule: https://community.languagetool.org/rule/show/SENTENCE_WHITESPACE?lang=en-US
Category: TYPOGRAPHY
// good | ||
conn, err := net.Dial("tcp", "localhost:80") | ||
if err != nil { | ||
if conn, err := net.Dial("tcp", "localhost:80"); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[LanguageTool] reported by reviewdog 🐶
Possible typo: you repeated a whitespace (WHITESPACE_RULE)
Suggestions:
Rule: https://community.languagetool.org/rule/show/WHITESPACE_RULE?lang=en-US
Category: TYPOGRAPHY
// good | ||
conn, err := net.Dial("tcp", "localhost:80") | ||
if err != nil { | ||
if conn, err := net.Dial("tcp", "localhost:80"); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[LanguageTool] reported by reviewdog 🐶
Don't put a space after the opening parenthesis (COMMA_PARENTHESIS_WHITESPACE)
Suggestions: {
Rule: https://community.languagetool.org/rule/show/COMMA_PARENTHESIS_WHITESPACE?lang=en-US
Category: TYPOGRAPHY
// handle error | ||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[LanguageTool] reported by reviewdog 🐶
Don't put a space after the opening parenthesis (COMMA_PARENTHESIS_WHITESPACE)
Suggestions: {
Rule: https://community.languagetool.org/rule/show/COMMA_PARENTHESIS_WHITESPACE?lang=en-US
Category: TYPOGRAPHY
// handle error | ||
} else { | ||
// use the conn |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[LanguageTool] reported by reviewdog 🐶
After 'the', do not use a verb. Make sure that the spelling of 'conn' is correct. If 'conn' is the first word in a compound adjective, use a hyphen between the two words. Note: This error message can occur if you use a verb as a noun, and the word is not a noun in standard English. (A_INFINITIVE[1])
Rule: https://community.languagetool.org/rule/show/A_INFINITIVE?lang=en-US&subId=1
Category: GRAMMAR
// handle error | ||
} else { | ||
// use the conn |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[LanguageTool] reported by reviewdog 🐶
Don't put a space before the closing parenthesis (COMMA_PARENTHESIS_WHITESPACE)
Suggestions: }
Rule: https://community.languagetool.org/rule/show/COMMA_PARENTHESIS_WHITESPACE?lang=en-US
Category: TYPOGRAPHY
|
||
```go | ||
// bad | ||
if conn, err := net.Dial("tcp", "localhost:80"); err != nil { | ||
conn, err := net.Dial("tcp", "localhost:80") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[LanguageTool] reported by reviewdog 🐶
Add a space between sentences (SENTENCE_WHITESPACE)
Suggestions: Dial
Rule: https://community.languagetool.org/rule/show/SENTENCE_WHITESPACE?lang=en-US
Category: TYPOGRAPHY
// bad | ||
if conn, err := net.Dial("tcp", "localhost:80"); err != nil { | ||
conn, err := net.Dial("tcp", "localhost:80") | ||
if err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[LanguageTool] reported by reviewdog 🐶
Don't put a space after the opening parenthesis (COMMA_PARENTHESIS_WHITESPACE)
Suggestions: {
Rule: https://community.languagetool.org/rule/show/COMMA_PARENTHESIS_WHITESPACE?lang=en-US
Category: TYPOGRAPHY
} | ||
// use the conn |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[LanguageTool] reported by reviewdog 🐶
After 'the', do not use a verb. Make sure that the spelling of 'conn' is correct. If 'conn' is the first word in a compound adjective, use a hyphen between the two words. Note: This error message can occur if you use a verb as a noun, and the word is not a noun in standard English. (A_INFINITIVE[1])
Rule: https://community.languagetool.org/rule/show/A_INFINITIVE?lang=en-US&subId=1
Category: GRAMMAR
Description:
This PR update the coding guideline and create 2 new section to cover the following things:
Please refer to this task ticket for this PR:
https://github.com/vdaas/vald/projects/3#card-44245759
Related Issue:
How Has This Been Tested?:
Environment:
Types of changes:
Changes to Core Features:
Checklist: